Use warnings.warn instead of logger.warning #321

mpenkov · 2019-05-28T12:29:47Z

piskvorky · 2019-05-28T21:18:13Z

smart_open/smart_open_lib.py

@@ -382,7 +382,7 @@ def open(

 def smart_open(uri, mode="rb", **kw):
    """Deprecated, use smart_open.open instead."""
-    logger.warning('this function is deprecated, use smart_open.open instead')
+    warnings.warn('this function is deprecated, use smart_open.open instead')


Can we link to the smart_open => open migration instructions here? This "use smart_open.open instead" is a massive ask, unless it's a 1:1 transparent swap without any potential issues (which I don't think it is).

Let's inform the user more clearly what are the expected tradeoffs / benefits of such change, and what their migration and potential incompatibilities look like.

piskvorky

Nicely done!

Submitting a few minor rewording suggestions.

README.rst

piskvorky · 2019-05-29T10:31:39Z

README.rst

+Since 1.8.1, there is a `smart_open.open` function that replaces `smart_open.smart_open`.
+The new function offers several advantages over the old one:
+
+- 100% compatible with the built-in open function (aka io.open): it accepts all


Suggested change

- 100% compatible with the built-in open function (aka io.open): it accepts all

- 100% compatible with the built-in `open` function (aka `io.open`): it accepts all

piskvorky · 2019-05-29T10:32:11Z

README.rst

+
+- 100% compatible with the built-in open function (aka io.open): it accepts all
+  the parameters that the built-in open accepts.
+- Default open mode is now "r", the same as for the built-in open


Suggested change

- Default open mode is now "r", the same as for the built-in open

- The default `smart_open.open` mode is now "r", the same as for the built-in `open` (`smart_open.smart_open` default used to be "rb").

piskvorky · 2019-05-29T10:32:35Z

README.rst

+- Default open mode is now "r", the same as for the built-in open
+- Fully documented keyword parameters (try `help("smart_open.open")`)
+
+These instructions will help you migrate to the new function painlessly.


Suggested change

These instructions will help you migrate to the new function painlessly.

The instructions below will help you migrate to the new function painlessly.

piskvorky · 2019-05-29T10:33:53Z

smart_open/smart_open_lib.py

+
+    warnings.warn(
+        'This function is deprecated, use smart_open.open instead. '
+        'See the migration notes for details: %s' % _MIGRATION_NOTES_URL


Suggested change

'See the migration notes for details: %s' % _MIGRATION_NOTES_URL

'See the migration instructions: %s' % _MIGRATION_NOTES_URL

Co-Authored-By: Radim Řehůřek <me@radimrehurek.com>

piskvorky

@mpenkov something went wrong with your edits.

piskvorky · 2019-05-31T07:55:23Z

README.rst

+  annual/monthly_rain/2012.monthly_rain.nc 13
+
+
+Migrating to the New `open` Function


@mpenkov what happened here? I remember commenting on this line in a review, and you fixing it. But now it's back in the master README.

piskvorky · 2019-05-31T07:58:58Z

README.rst

+
+- `profile_name`
+
+**The `profile_name` parameter has been removed.**


Something is wrong with the backticks. They have no effect in this section (look like normal text when rendered), and this line actually renders them as literal backticks:

I think that's just the way github renders RST. I don't know what we can do about that.

Aaah, this is RST, not Markdown! That explains it.

Literal blocks in RST use double-backticks IIRC, not single-backtick.

Do we need to use RST though? MD easier? Or is it something to do with PyPI relying on RST…

Yes, it's because of PyPI

piskvorky · 2019-05-31T08:03:47Z

README.rst

+See `help("smart_open.open")` for the full list of acceptable parameter names,
+or view the help online `here <https://github.com/RaRe-Technologies/smart_open/blob/master/help.txt>`__.
+
+If you pass an invalid parameter names, the `open` function will warn you about it.


typo:

If you pass an invalid parameter name, the `open` function will warn you about it.

mpenkov added 2 commits May 28, 2019 21:26

updated release script to refresh help.txt

6433b62

use warnings.warn instead of logger.warning

88f213f

piskvorky reviewed May 28, 2019

View reviewed changes

mpenkov added 6 commits May 29, 2019 10:36

add migration notes

948838b

update README.rst to pass doctests

c57ae73

add doctest to release script

9a85d05

link to migration notes from docstring and warning

88618e0

fix RST formatting

beb3857

minor fixup

5877193

piskvorky requested changes May 29, 2019

View reviewed changes

mpenkov and others added 2 commits May 29, 2019 21:06

Update README.rst

5485c86

Co-Authored-By: Radim Řehůřek <me@radimrehurek.com>

respond to review comments

69fa8de

mpenkov merged commit 2a5b16d into piskvorky:master May 29, 2019

mpenkov deleted the warn branch May 29, 2019 14:17

piskvorky reviewed May 31, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use warnings.warn instead of logger.warning #321

Use warnings.warn instead of logger.warning #321

mpenkov commented May 28, 2019

piskvorky May 28, 2019 •

edited

Loading

piskvorky left a comment

piskvorky May 29, 2019

piskvorky May 29, 2019

piskvorky May 29, 2019

piskvorky May 29, 2019

piskvorky left a comment

piskvorky May 31, 2019 •

edited

Loading

piskvorky May 31, 2019

mpenkov May 31, 2019

piskvorky May 31, 2019 •

edited

Loading

mpenkov May 31, 2019

piskvorky May 31, 2019 •

edited

Loading

	- 100% compatible with the built-in open function (aka io.open): it accepts all
	- 100% compatible with the built-in `open` function (aka `io.open`): it accepts all

	- Default open mode is now "r", the same as for the built-in open
	- The default `smart_open.open` mode is now "r", the same as for the built-in `open` (`smart_open.smart_open` default used to be "rb").

	These instructions will help you migrate to the new function painlessly.
	The instructions below will help you migrate to the new function painlessly.

	'See the migration notes for details: %s' % _MIGRATION_NOTES_URL
	'See the migration instructions: %s' % _MIGRATION_NOTES_URL

		annual/monthly_rain/2012.monthly_rain.nc 13


		Migrating to the New `open` Function


		- `profile_name`

		The `profile_name` parameter has been removed.

Use warnings.warn instead of logger.warning #321

Use warnings.warn instead of logger.warning #321

Conversation

mpenkov commented May 28, 2019

piskvorky May 28, 2019 • edited Loading

Choose a reason for hiding this comment

piskvorky left a comment

Choose a reason for hiding this comment

piskvorky May 29, 2019

Choose a reason for hiding this comment

piskvorky May 29, 2019

Choose a reason for hiding this comment

piskvorky May 29, 2019

Choose a reason for hiding this comment

piskvorky May 29, 2019

Choose a reason for hiding this comment

piskvorky left a comment

Choose a reason for hiding this comment

piskvorky May 31, 2019 • edited Loading

Choose a reason for hiding this comment

piskvorky May 31, 2019

Choose a reason for hiding this comment

mpenkov May 31, 2019

Choose a reason for hiding this comment

piskvorky May 31, 2019 • edited Loading

Choose a reason for hiding this comment

mpenkov May 31, 2019

Choose a reason for hiding this comment

piskvorky May 31, 2019 • edited Loading

Choose a reason for hiding this comment

piskvorky May 28, 2019 •

edited

Loading

piskvorky May 31, 2019 •

edited

Loading

piskvorky May 31, 2019 •

edited

Loading

piskvorky May 31, 2019 •

edited

Loading